[Refactor] Remove redundant StageDeployConfig fields, delegate to vLLM defaults#3128
[Refactor] Remove redundant StageDeployConfig fields, delegate to vLLM defaults#3128gcanlin wants to merge 18 commits into
Conversation
…M defaults Signed-off-by: gcanlin <canlinguosdu@gmail.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
I found that this field is changing the default value from vllm. And it produces the different behavior before stage config refactoring. So I think we don't need to set the default on vllm-omni side. vLLM has the selector method for this field:
|
I completely agree with your point of view. I believe that the default parameters should be consistent with vLLM; otherwise, performance will be affected by the inconsistency in default parameters. |
|
If it involves modifying deployment parameters, I suggest testing with nightly-test. |
|
cc @yenuo26 |
|
|
||
|
|
||
| DEPLOY_CONFIGS_DIR = Path(__file__).parent.parent / "deploy" | ||
| DEPLOY_CONFIGS_DIR = Path(__file__).resolve().parents[4] / "vllm_omni" / "deploy" |
There was a problem hiding this comment.
maybe you can use get_deploy_config_path in tests/helpers/stage_config.py
| "stage_args": { | ||
| 0: {"engine_args.enable_prefix_caching": True}, | ||
| "stages": { | ||
| 0: {"enable_prefix_caching": True}, |
There was a problem hiding this comment.
Does this not support passing arguments from the command line?
There was a problem hiding this comment.
Sounds good. But would be better to unify it in a follow-up PR.
Signed-off-by: gcanlin <canlinguosdu@gmail.com>
| stage_id: int | ||
| max_num_seqs: int = 64 | ||
| gpu_memory_utilization: float = 0.9 | ||
| tensor_parallel_size: int = 1 |
There was a problem hiding this comment.
For the requirement where fields defined in deploy YAML are selectively overridden,
these fields still need to be retained as a whitelist to preserve the override semantics.
The core issue here is handling default values. A more consistent approach would be to treat
all values as optional (i.e., default to None), and introduce a filtering
step in deploy_create_from_registry(), such as:
explicit_overrides = {k: v for k, v in override_fields.items() if v is not None}
This ensures that only values explicitly provided in the deploy YAML are included in the resulting StageConfig, while missing fields are left unset and handled by downstream default resolution.
With this design, as discussed in #3162:
- For LLM stages, missing fields will fall back to the default values defined in vLLM's
EngineArgs. - For diffusion stages, missing fields will be handled by
_create_default_diffusion_stage_cfg,
which provides safe defaults.
This unifies the override behavior while delegating default resolution to the appropriate
downstream layer.
|
lgtm, the CI failure may not relate to this PR, resolve conflicts please |
|
Fix conflicts and pre-commit problems please |
PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.
Purpose
Remove fields from StageDeployConfig that duplicate vLLM EngineArgs defaults:
These fields now flow through engine_extras and inherit vLLM's hardware-specific defaults. Existing YAML configs remain compatible as unrecognized fields automatically enter engine_extras.
Retained fields with vllm-omni specific behavior:
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model. Please runmkdocs serveto sync the documentation editions to./docs.BEFORE SUBMITTING, PLEASE READ https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md (anything written below this line will be removed by GitHub Actions)